-
Notifications
You must be signed in to change notification settings - Fork 855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NETBEANS-1396] Missing code completion and Javadocs in maven projects with classifier #1548
Conversation
I created NETBEANS-3296 to track all the errors related to using the JavaFX Javadoc and Sources in NetBeans. An identical copy of the issue description is on GitHub at jgneff/netbeans-doc-src. |
Thomas asked me to test this pull request. Below are the results of my tests using the Non-Modular Maven project of the JavaFX samples repository. Once we resolve the issues with this first project, I can test the other sample projects described in NETBEANS-3296. I ran the tests with OpenJDK 13.0.1 on Ubuntu 16.04.6 LTS. I did not install the nb-javac plug-in so as to avoid the exceptions it encounters. For the Java Platform Javadoc, I left the default entry created by NetBeans, which lists only the docs directory of the JDK. Before downloading the JavaFX Javadoc and Sources, neither were available to NetBeans, as expected. I right-clicked the Dependencies node in the Project window and selected Download Javadoc. The Javadoc artifacts were downloaded to the local Maven cache under the directory ~/.m2/repository/org/openjfx.
The Javadoc badges appeared on those dependencies without the -linux.jar platform classifier. To my surprise, the Javadoc then displayed perfectly even before attaching the Sources! I had previously thought that NetBeans always got the Javadoc from the comments in the source code. The Javadoc window showed the full description when selecting the The full description was displayed when selecting the Clicking the toolbar button to show the documentation in an external Web browser opened my browser to the correct location. Furthermore, the Javadoc for the Java Platform was found with the default entry created by NetBeans. Previously, you had to edit the Platform Javadoc and add the specific Java module directories for the information to display correctly. Clicking through to the Sources showed only the Compiled Code, as expected. Next, I right-clicked the Dependencies node in the Project window and selected Download Sources. The Sources artifacts were downloaded to the local Maven cache under the directory ~/.m2/repository/org/openjfx.
The Sources badges appeared on those dependencies without the -linux.jar platform qualifier. Here I started to see some unexpected behavior. Selecting the Selecting the To see what's happening, I deleted the Javadoc from the Maven cache and left just the Sources. I closed and reopened the project. Now we see the same problems. The The Ant projects in NetBeans 11.2, though, are able to show the Javadoc for this method using only the comments in the source code, as shown in the screenshot below. I expected the Maven project to provide the same information. Note that you can tell whether the Javadoc and Sources are available by looking at whether the corresponding toolbar buttons are enabled. In the images above and below, only the Sources button is enabled. No Javadoc is attached. The Navigating to the /**
* Specify the scene to be used on this stage.
*/
@Override final public void setScene(Scene value) {
Toolkit.getToolkit().checkFxUserThread();
super.setScene(value);
}
/**
* {@inheritDoc}
*/
@Override public final void show() {
super.show();
} How should the Javadoc be displayed now? I'm guessing at the following algorithm based on the results of the all tests I've been doing here and for NETBEANS-3296.
|
The changes look sane. +1 for merging this one. |
I would prefer fixing the problems that occur after downloading the Sources: the Javadoc description disappears and the source navigation fails. I know, easy for me to say. 😃 We could, however, fix these in pieces and open new issues for those problems. I just ran some more tests to make sure that this pull request doesn't break the technique I use to work on the JavaFX Graphics module in NetBeans. The work-around is to create an Ant-based project and add the individual JAR files to the Module or Compile Classpath, attach the appropriate Javadoc and Sources directories to each JAR file, remove the Javadoc and Sources from the Global Library, and uninstall the nb-javac plug-in. That still works as described in the Ant Non-Modular and Ant-Modular sections of NETBEANS-3296. |
I'd say merge this in resolve the rest as separate issues. |
Thank you so much for the detailed writeup @jgneff! Those are some good test cases, I will investigate. I'm fine with merging this now, I'll open another PR for the disappearing javadoc after attaching sources. Is there already an issue for this? I'm not sure I can fix the source navigation though, those parts of the code looked very compiler-y to me and I might need some help there. |
Thanks, Thomas. I'm fine with merging this now, too. I will create a new sub-task of NETBEANS-3296 for the problems I found in my tests. I'm still trying to keep all of these issues together under the one main goal of attaching and using the Javadoc and Sources of a library.
It looks as if once you add the Sources, they take precedence over the HTML for displaying the Javadoc (and fail). Maybe that precedence could be swapped, using the Sources only when the Javadoc is not available for a selected item. I have not yet found any documentation on exactly how this is supposed to work. Have you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing the modification in RepositoryForBinaryQueryImpl.java
and reading
I would prefer fixing the problems that occur after downloading the Sources: the Javadoc description disappears and the source navigation fails.
I get a bad feeling. This query is used in other places - is there an explanation for the observations?
@@ -0,0 +1,360 @@ | |||
<!DOCTYPE HTML> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a license header into this file.
@@ -0,0 +1,367 @@ | |||
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a license header into this file.
@@ -0,0 +1,322 @@ | |||
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a license header into this file.
@@ -0,0 +1,406 @@ | |||
<!DOCTYPE HTML> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a license header into this file.
If that question is for me, just follow along in my tests to the point where, after adding the Sources, the Javadoc description disappears for the |
@jgneff your comment was my trigger. The question is directed to the author of this PR, the modified query has a larger userbase, than just the javadoc access, so potential breakage needs to be considered harder, than a local regression. |
@matthiasblaesing I am not sure yet, I need more time to investigate. Can I tag you when I find the underlying reason? I'm trying hard not to change existing code, but the logic used to find the javadoc is spread out and very stateful. |
After some digging, here are my findings. The algorithm to find the javadoc goes as follows:
I think the order is sensible, changing it would probably be too disruptive. I traced the execution after requesting javadoc for About That's where I stopped, the next step would probably be to see what's so special about Stage.java that javac can't parse its type references. @matthiasblaesing I did not check why the Ant project works, but fixing the javac errors has to be done anyway. While looking through the source I also found two other places that deal with javadoc that need to be adapted to javadoc 9+ output (namely HTMLJavadocParser::parseClass and JavadocHelper.TextStream). I'd like to consolidate the logic a bit, I think I have a way that doesn't break any public API. But leaving those places in for now shouldn't have a huge negative impact. I'm for merging this PR and I'll do a follow up PR when time permits. |
Thank you for the great research, Thomas. Thanks especially for documenting how this is supposed to work according to the code. So it's the opposite of my guess—Sources first, then Javadoc. That makes sense, as many of the Javadoc comments are not published in their Javadoc HTML. I also tried to figure out what's supposed to happen, but by comparing NetBeans with IntelliJ IDEA when they both appear to be working. I posted my comparison as Apache NetBeans & IntelliJ IDEA. I found the comparison interesting, but I don't think you'll see anything you don't already know. |
The point about attached Javadoc substituting missing comments in the attached Source is subtle, great find! I revised the algorithm in my comment to reflect this. I verified that this is supposed to be happening with |
Thanks, Thomas. That's a good update to the algorithm description. There are quite a few subtleties in all of this. I added one more test to demonstrate the precedence for both IDEs and put the results in a new section called Precedence. That test finally shows an important difference between the two, and I prefer how NetBeans does it. I also added a section at the end called JavaFX library to show how I set up both IDEs to use the Javadoc and Sources. The new sections are added to my Apache NetBeans & IntelliJ IDEA comparison. |
Just for future reference, the algorithm doesn't work for the new |
Also for future reference, the new Notice that all the keys are missing in the table below for |
…ependencies with classifiers OpenJFX uses Maven classifiers for platform dependent code, i.e. javafx-graphics-13.jar does not contain any code, but it depends through build profiles on javafx-graphics-13-linux.jar. There is however no javafx-graphics-13-linux-javadoc.jar, only a javafx-graphics-13-javadoc.jar, which NetBeans doesn't find. This patch lets NetBeans fall back to the main javadoc jar if there is none associated with the one with the classifier. The contents of the javadoc jars of OpenJFX are prefixed by module name, which is different from the default output of the maven-javadoc-plugin. This patch does some bytecode reading to still find the correct .html file. Fix RepositoryForBinaryQueryImplTest::testResultChanging which was broken because of a missing test resource.
@jgneff Would you mind testing it again? I rebased onto the master branch and now source navigation and |
@zimmi, sure! I should have time to test it early this week. Next time, though, could you add incremental commits instead of a forced push? That would save time for anyone testing the pull request. See the JavaFX CONTRIBUTING guidelines for an example of a good general policy:
|
You are right, sorry for that! I'll merge master in the future. |
@jgneff that's actually different to NetBeans' own contribution guidelines! Such a change may not be a bad thing, but maybe something to discuss on dev@ rather than in a PR?! |
I forgot to switch the Java Platform for the project from JDK 8 to JDK 13. As soon as I do that, it's broken again. Sorry for the false hope. I might have some time this week to look into the nb-javac error, maybe I can come up with something. This PR is still good to go as is from my perspective though, the errors existed previously as well. |
Thanks for the update, Thomas. I'm all set up to test this morning, anyway, so I'll check for any changes without the nb-javac plug-in on JDK 13.0.1 and the early-access build of JavaFX 14-ea+2. |
For these tests, I repeated my previous tests and the Apache NetBeans & IntelliJ IDEA comparison tests using an early-access build of JavaFX 14. I found two differences. Previous testsThe first difference is due to using the early-access build of JavaFX 14 instead of the released version 13.0.1. With JavaFX 13.0.1, showing the documentation in an external Web browser opens the Javadoc page at the following location: With the early-access build of JavaFX 14-ea+2, showing the documentation in an external Web browser opens an error page at the following location: The error page contains:
It seems to be a simple URL-encoding problem, but I couldn't find a different encoding of the plus sign that worked. I would suggest there's a problem in the JavaFX version string, except that it's part of the Semantic Versioning customs:
Comparison testsThe second difference is compared to the working Ant-based tests. In Ant-based projects set up to work properly, the Javadoc navigation is the same whether you're selecting a method where it's used in an application, or you're selecting the same method where it's defined in a library. In this Non-Modular Maven project, the toolbar button to open the Javadoc in an external Web browser is enabled when you select a method where it's used, but the button becomes disabled when you select the same method where it's defined. This occurs for both JavaFX 13.0.1 and JavaFX 14-ea+2. This is not a new problem—just something I had not noticed in my earlier tests. I'm fine with this pull request being merged. We can open new issues for the problems I found. @zimmi, for what it matters, I'm fine now if you rewrite commits. I finally understand that NetBeans merges the pull request history directly into master as-is, so it really does matter what's there. My limited experience is with OpenJDK projects, which forbid forced pushes, discard the pull request history, and add only a single fast-forward commit to master. I assumed that all projects worked in a similar manner, so I couldn't understand why an author would even bother to tidy up the pull request history. I think I understand better now, and I'm sorry for my ignorant assumptions. |
Thank you for testing it again, John! There are many small things to improve, it's great to have them all written down. I also have two smaller things regarding the toolbar button:
Spending a bit more time in the debugger, I found that replacing |
Could that be the reason |
Hi, @neilcsmith-net and @matthiasblaesing. I'm not sure of the status for this PR, is this mergeable ? need feedback or still work in progress ? |
@ebarboni personally don't know - although looks like @lkishalmi has tested and recommends merging? Good to get in in principle - it's affecting me on a project at the moment! |
I've only checked this PR at the early stage, looked good and in fact it is not being merged blocks a few of my outstanding fixes for Gradle vs JavaFx documentation. I'm kind of +0 merging it at this phase of the release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+0 as of the timing.
I didn't do a code review, but I did test the fix and document the results rather thoroughly two times—on Nov 7, 2019 and again on Nov 21, 2019. Based on the test results, I recommended that it be merged at the time. It's easy to follow along and repeat my tests, if anyone wants to test it on the latest NetBeans. I also created a repository comparing NetBeans with the competition on this feature. NetBeans wins the comparison, in my opinion, but only if these bugs can get out of the way. So for me, the sooner this is merged, the better! That will make it one down, ten more to go. |
going to merge. |
Thank you for merging! I got a new job and didn't have much time to dedicate to this. I might come back to it when time permits, there are still some improvements / fixes left for full JavaFX support with Maven. |
Edit: No need to rush this for 11.2.
This solves NETBEANS-1396 and NETBEANS-2197 with the goal of supporting JavaFX / OpenJFX Javadoc and
Go to Declaration
. I split the solution into two commits for easier review:javafx-graphics-13.jar
does not contain any code, but it depends through build profiles onjavafx-graphics-13-linux.jar
. There is however nojavafx-graphics-13-linux-javadoc.jar
, only ajavafx-graphics-13-javadoc.jar
, which NetBeans doesn't find. This PR lets NetBeans fall back to the main javadoc jar if there is none associated with the one with the classifier..html
file.To link the discussions together: openjfx/openjfx-docs#44, openjfx/javadoc#6 and a stackoverflow question.